-
Notifications
You must be signed in to change notification settings - Fork 204
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add coal and gas CC technologies #1200
base: main
Are you sure you want to change the base?
Add coal and gas CC technologies #1200
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @yerbol-akhmetov, great work! 😄
Have added some comments which mainly relate to work with the configuration parameters. Could you please have a look? (sorry for jumping in, Davide has just notified that he is busy this week)
config.default.yaml
Outdated
@@ -358,6 +358,7 @@ costs: | |||
year: 2030 | |||
version: v0.6.2 | |||
discountrate: [0.071] #, 0.086, 0.111] | |||
scenario: "Moderate" # "Advanced", "Moderate", or "Conservative" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be great to add a comment on what exactly is meant by scenario
here, as we have quite many of them in the config 😄 Probably, adjustment of the naming can help, as well: like tech_scenario
or costs_scenario
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review @ekatef. I will probably remove these changes. As it needs to be applied with @danielelerede-oet and @finozzifa PR on cost assumption integration. Being under costs section scenario is plan to mean cost scenario
. But if it can lead to misunderstanding then I agree we can name it as cost_scenario
.
@@ -508,6 +509,9 @@ sector: | |||
network: false # ALWAYS FALSE for now (NOT USED) | |||
network_data: GGIT # Global dataset -> 'GGIT' , European dataset -> 'IGGIELGN' | |||
network_data_GGIT_status: ["Construction", "Operating", "Idle", "Shelved", "Mothballed", "Proposed"] | |||
gas_NGCC: false # if true, Natural Gas 2-on-1 Combined Cycle (F-Frame) plant is added | |||
CC: false # enable gas carbon capture technologies |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My feeling is that it would be great to keep harmonise using upper and lowercase across the code: we have lower-cased cc
for industry and co-generation here, while upper-case is used for steam methane reforming and thermal power plants in this PR. Can we align that somehow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, my thought was to change everywhere to uppercase CC
. As it is strange that in SMR CC
it is uppercase, while just cc
is lowercase. I can make those changes within this PR if you agree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally agree 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review, @ekatef. I will apply your suggestions into the code. However, this PR will still be draft as it will require new costs assumptions first being added to PyPSA/technology data and PyPSA-Earth.
config.default.yaml
Outdated
@@ -358,6 +358,7 @@ costs: | |||
year: 2030 | |||
version: v0.6.2 | |||
discountrate: [0.071] #, 0.086, 0.111] | |||
scenario: "Moderate" # "Advanced", "Moderate", or "Conservative" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review @ekatef. I will probably remove these changes. As it needs to be applied with @danielelerede-oet and @finozzifa PR on cost assumption integration. Being under costs section scenario is plan to mean cost scenario
. But if it can lead to misunderstanding then I agree we can name it as cost_scenario
.
@@ -508,6 +509,9 @@ sector: | |||
network: false # ALWAYS FALSE for now (NOT USED) | |||
network_data: GGIT # Global dataset -> 'GGIT' , European dataset -> 'IGGIELGN' | |||
network_data_GGIT_status: ["Construction", "Operating", "Idle", "Shelved", "Mothballed", "Proposed"] | |||
gas_NGCC: false # if true, Natural Gas 2-on-1 Combined Cycle (F-Frame) plant is added | |||
CC: false # enable gas carbon capture technologies |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, my thought was to change everywhere to uppercase CC
. As it is strange that in SMR CC
it is uppercase, while just cc
is lowercase. I can make those changes within this PR if you agree.
Thanks for going trough the comments @yerbol-akhmetov. I just remembered that we have promised a preliminary review. So, I have tried to provide some initial feedback, though I absolutely agree that the final design depends also on the particular implementation of the technologies parameters in the technologies database. My general impression is that PR introduces very interesting features into the model. Great work, and looking forward to see it being completed. |
Thanks for the revisions @yerbol-akhmetov. I recognise that the PR is still work in progress, but I think it's on a good track. Have lifted |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm commenting here as I've been pinged; I've scanned the contribution and looks quite promising!
Do you envision updates?
As minor comments, I'd support Katia's comment to remove the line "scenario" in config.default.
Moreover, to test the functionality of the model, I'd advise to revise the file test/config.test1.yaml to enable the sector-coupled technologies you proposed to ensure they get tested in the CI
Thanks for revising it, @davide-f. The only change that I would like to make is to make sure these technologies are added only is costs file contains information about these techs, and also provide log that tech is not in costs file is not present. This way we make sure the error will not occur and user will need to use proper costs. The costs PR is not yet in pypsa-earth though. Adding if condition can help to prevent errors. What do you think? In short, I mean adding if conditions:
|
Thanks, @davide-f. Regarding inclusion to test CI, I will revise it to include added techs there. But it will require costs file by NREL ATB to fully test it. Otherwise if we implement |
Changes proposed in this Pull Request
Good day. This PR aims to make coal and gas CC technologies available for selection in the config. Currently, it is a draft PR.
@davide-f, @danielelerede-oet, @GbotemiB
I am planning to add gas CC technologies and also test with costs.
Checklist
config.default.yaml
andconfig.tutorial.yaml
.test/
(note tests are changing the config.tutorial.yaml)doc/configtables/*.csv
and line references are adjusted indoc/configuration.rst
anddoc/tutorial.rst
.doc/release_notes.rst
is amended in the format of previous release notes, including reference to the requested PR.